Skip to content

feat: implement pallet_msa::withdraw_tokens extrinsic #2402

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Open
wants to merge 36 commits into
base: main
Choose a base branch
from
Open

feat: implement pallet_msa::withdraw_tokens extrinsic #2402

wants to merge 36 commits into from

Conversation

JoeCap08055
Copy link
Collaborator

@JoeCap08055 JoeCap08055 commented May 12, 2025 •
edited
Loading

Goal

The goal of this PR is to implement an initia withdraw_tokens extrinsic that allows MSAs holding tokens to authorize their tokens to be withdrawn to a specified account.

Closes #2388

Discussion

Checklist

  • Updated Pallet Readme?
  • Updated js/api-augment for Custom RPC APIs?
  • Design doc(s) updated?
  • Unit Tests added?
  • e2e Tests added?
  • Benchmarks added?
  • Spec version incremented?

@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels May 12, 2025
@github-actions github-actions bot removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels May 13, 2025
@codecov Codecov
Copy link

codecov bot commented May 13, 2025 •
edited
Loading

Codecov Report

Attention: Patch coverage is 97.16312% with 4 lines in your changes missing coverage. Please review.

Files with missing lines Patch % Lines
pallets/msa/src/lib.rs 97.24% 3 Missing ⚠️
pallets/msa/src/types.rs 96.87% 1 Missing ⚠️
Files with missing lines Coverage Δ
pallets/msa/src/types.rs 97.74% <96.87%> (-0.28%) ⬇️
pallets/msa/src/lib.rs 93.15% <97.24%> (+0.40%) ⬆️
🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels May 13, 2025
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels May 13, 2025
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label May 13, 2025
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels May 13, 2025
aramikm
aramikm reviewed May 13, 2025
View reviewed changes
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels May 14, 2025
@JoeCap08055 JoeCap08055 marked this pull request as ready for review May 14, 2025 16:33
@JoeCap08055 JoeCap08055 requested a review from wilwade as a code owner May 14, 2025 16:33
@JoeCap08055 JoeCap08055 force-pushed the feat/withdraw-tokens-from-msa branch from 22fe1f5 to 1308dac Compare May 28, 2025 14:52
@github-actions github-actions bot removed the metadata-changed Metadata has changed since the latest full release label May 28, 2025
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label May 28, 2025
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels May 28, 2025
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented and removed metadata-changed Metadata has changed since the latest full release labels May 28, 2025
@github-actions github-actions bot removed metadata-changed Metadata has changed since the latest full release metadata-version-not-incremented Metadata has changed since the latest full release, but the version has not been incremented labels May 28, 2025
@github-actions github-actions bot added the metadata-changed Metadata has changed since the latest full release label May 28, 2025
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels May 28, 2025
@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels May 28, 2025
aramikm
aramikm reviewed May 28, 2025
View reviewed changes
Copy link
Collaborator

@aramikm aramikm left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Great effort but there are some blockers.

pallets/msa/src/weights.rs Outdated
// Measured: `0`
// Estimated: `0`
// Minimum execution time: 4_000_000 picoseconds.
Weight::from_parts(8_000_000, 0)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

DB read write don't match. Need to run benchmarks for accurate weights.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah, right, benchmarks. Will run.

@github-actions github-actions bot added metadata-changed Metadata has changed since the latest full release and removed metadata-changed Metadata has changed since the latest full release labels May 29, 2025
shannonwells
shannonwells approved these changes May 30, 2025
View reviewed changes
Copy link
Collaborator

@shannonwells shannonwells left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I had just the nitpick comment but nothing blocking. Lots of good work!

pallets/msa/src/lib.rs
/// * [`Error::ProofHasExpired`]
/// * [`Error::SignatureAlreadySubmitted`]
///
pub fn check_signature(
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This naming is confusing between this and verify_signature; I suggest check_against_signature_registry or something else more specific, since it doesn't examine the signature itself but makes sure it's not already registered and that it's ready but not expired.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mattheworris mattheworris mattheworris left review comments

@aramikm aramikm aramikm left review comments

@shannonwells shannonwells shannonwells approved these changes

@wilwade wilwade Awaiting requested review from wilwade wilwade is a code owner

@enddynayn enddynayn Awaiting requested review from enddynayn enddynayn was automatically assigned from frequency-chain/pr-reviewers

@claireclark1 claireclark1 Awaiting requested review from claireclark1 claireclark1 was automatically assigned from frequency-chain/pr-reviewers

At least 3 approving reviews are required to merge this pull request.

Assignees
No one assigned
Labels
metadata-changed Metadata has changed since the latest full release
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Tokens held by MSA-owned accounts can be withdrawn
5 participants
@JoeCap08055 @shannonwells @wilwade @mattheworris @aramikm